fix(readonly): better handling of ACL on read-only files on windows

Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>

diff --git a/src/common/filesystembase.cpp b/src/common/filesystembase.cpp
index e2b4837f43620c5f78815492321487118048817d..cb83dc576511c9a9684979938e158656efb9efd5 100644 (file)
--- a/src/common/filesystembase.cpp
+++ b/src/common/filesystembase.cpp
@@ -35,6 +35,8 @@
 #include <winbase.h>
 #include <fcntl.h>
 #include <io.h>
+#include <securitybaseapi.h>
+#include <sddl.h>
 #endif
 
 namespace OCC {
@@ -148,6 +150,12 @@ void FileSystem::setFileReadOnly(const QString &filename, bool readonly)
         }
     }
 
+    if (!readonly) {
+        // current read-only folder ACL needs to be removed from files also when making a folder read-write
+        // we currently have a too limited set of authorization for files when applying the restrictive ACL for folders on the child files
+        setAclPermission(filename, FileSystem::FolderPermissions::ReadWrite, false);
+    }
+
     return;
 #endif
     QFile file(filename);
@@ -689,6 +697,157 @@ QString FileSystem::pathtoUNC(const QString &_str)
     }
     return QStringLiteral(R"(\\?\)") + str;
 }
+
+bool FileSystem::setAclPermission(const QString &unsafePath, FolderPermissions permissions, bool applyAlsoToFiles)
+{
+    SECURITY_INFORMATION info = DACL_SECURITY_INFORMATION;
+    std::unique_ptr<char[]> securityDescriptor;
+    auto neededLength = 0ul;
+
+    const auto path = longWinPath(unsafePath);
+
+    const auto safePathFileInfo = QFileInfo{path};
+
+    if (!GetFileSecurityW(path.toStdWString().c_str(), info, nullptr, 0, &neededLength)) {
+        const auto lastError = GetLastError();
+        if (lastError != ERROR_INSUFFICIENT_BUFFER) {
+            qCWarning(lcFileSystem) << "error when calling GetFileSecurityW" << path << lastError;
+            return false;
+        }
+
+        securityDescriptor.reset(new char[neededLength]);
+
+        if (!GetFileSecurityW(path.toStdWString().c_str(), info, securityDescriptor.get(), neededLength, &neededLength)) {
+            qCWarning(lcFileSystem) << "error when calling GetFileSecurityW" << path << GetLastError();
+            return false;
+        }
+    }
+
+    int daclPresent = false, daclDefault = false;
+    PACL resultDacl = nullptr;
+    if (!GetSecurityDescriptorDacl(securityDescriptor.get(), &daclPresent, &resultDacl, &daclDefault)) {
+        qCWarning(lcFileSystem) << "error when calling GetSecurityDescriptorDacl" << path << GetLastError();
+        return false;
+    }
+    if (!daclPresent || !resultDacl) {
+        qCWarning(lcFileSystem) << "error when calling DACL needed to set a folder read-only or read-write is missing" << path;
+        return false;
+    }
+
+    PSID sid = nullptr;
+    if (!ConvertStringSidToSidW(L"S-1-5-32-545", &sid))
+    {
+        qCWarning(lcFileSystem) << "error when calling ConvertStringSidToSidA" << path << GetLastError();
+        return false;
+    }
+
+    ACL_SIZE_INFORMATION aclSize;
+    if (!GetAclInformation(resultDacl, &aclSize, sizeof(aclSize), AclSizeInformation)) {
+        qCWarning(lcFileSystem) << "error when calling GetAclInformation" << path << GetLastError();
+        return false;
+    }
+
+    const auto newAclSize = aclSize.AclBytesInUse + sizeof(ACCESS_DENIED_ACE) + GetLengthSid(sid);
+    qCDebug(lcFileSystem) << "allocated a new DACL object of size" << newAclSize;
+
+    std::unique_ptr<ACL> newDacl{reinterpret_cast<PACL>(new char[newAclSize])};
+    if (!InitializeAcl(newDacl.get(), newAclSize, ACL_REVISION)) {
+        const auto lastError = GetLastError();
+        if (lastError != ERROR_INSUFFICIENT_BUFFER) {
+            qCWarning(lcFileSystem) << "insufficient memory error when calling InitializeAcl" << path;
+            return false;
+        }
+
+        qCWarning(lcFileSystem) << "error when calling InitializeAcl" << path << lastError;
+        return false;
+    }
+
+    if (permissions == FileSystem::FolderPermissions::ReadOnly) {
+        qCInfo(lcFileSystem) << path << "will be read only";
+
+        if (!AddAccessDeniedAceEx(newDacl.get(), ACL_REVISION, OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE,
+                                  FILE_DELETE_CHILD | DELETE | FILE_WRITE_DATA | FILE_WRITE_ATTRIBUTES | FILE_WRITE_EA | FILE_APPEND_DATA, sid)) {
+            qCWarning(lcFileSystem) << "error when calling AddAccessDeniedAce << path" << GetLastError();
+            return false;
+        }
+    }
+
+    if (permissions == FileSystem::FolderPermissions::ReadWrite) {
+        qCInfo(lcFileSystem) << path << "will be read write";
+    }
+
+    for (int i = 0; i < aclSize.AceCount; ++i) {
+        void *currentAce = nullptr;
+        if (!GetAce(resultDacl, i, &currentAce)) {
+            qCWarning(lcFileSystem) << "error when calling GetAce" << path << GetLastError();
+            return false;
+        }
+
+        const auto currentAceHeader = reinterpret_cast<PACE_HEADER>(currentAce);
+
+        if (permissions == FileSystem::FolderPermissions::ReadWrite && (ACCESS_DENIED_ACE_TYPE == (currentAceHeader->AceType & ACCESS_DENIED_ACE_TYPE))) {
+            qCWarning(lcFileSystem) << "AceHeader" << path << currentAceHeader->AceFlags << currentAceHeader->AceSize << currentAceHeader->AceType;
+            continue;
+        }
+
+        if (!AddAce(newDacl.get(), ACL_REVISION, i + 1, currentAce, currentAceHeader->AceSize)) {
+            const auto lastError = GetLastError();
+            if (lastError != ERROR_INSUFFICIENT_BUFFER) {
+                qCWarning(lcFileSystem) << "insufficient memory error when calling AddAce" << path;
+                return false;
+            }
+
+            if (lastError != ERROR_INVALID_PARAMETER) {
+                qCWarning(lcFileSystem) << "invalid parameter error when calling AddAce" << path << "ACL size" << newAclSize;
+                return false;
+            }
+
+            qCWarning(lcFileSystem) << "error when calling AddAce" << path << lastError << "acl index" << (i + 1);
+            return false;
+        }
+    }
+
+    SECURITY_DESCRIPTOR newSecurityDescriptor;
+    if (!InitializeSecurityDescriptor(&newSecurityDescriptor, SECURITY_DESCRIPTOR_REVISION)) {
+        qCWarning(lcFileSystem) << "error when calling InitializeSecurityDescriptor" << path << GetLastError();
+        return false;
+    }
+
+    if (!SetSecurityDescriptorDacl(&newSecurityDescriptor, true, newDacl.get(), false)) {
+        qCWarning(lcFileSystem) << "error when calling SetSecurityDescriptorDacl" << path << GetLastError();
+        return false;
+    }
+
+    if (safePathFileInfo.isDir() && applyAlsoToFiles) {
+        const auto currentFolder = safePathFileInfo.dir();
+        const auto childFiles = currentFolder.entryList(QDir::Filter::Files);
+        for (const auto &oneEntry : childFiles) {
+            const auto childFile = QDir::toNativeSeparators(path + QDir::separator() + oneEntry);
+
+            const auto &childFileStdWString = childFile.toStdWString();
+            const auto attributes = GetFileAttributes(childFileStdWString.c_str());
+
+                   // testing if that could be a pure virtual placeholder file (i.e. CfApi file without data)
+                   // we do not want to trigger implicit hydration ourself
+            if ((attributes & FILE_ATTRIBUTE_SPARSE_FILE) != 0) {
+                continue;
+            }
+
+            if (!SetFileSecurityW(childFileStdWString.c_str(), info, &newSecurityDescriptor)) {
+                qCWarning(lcFileSystem) << "error when calling SetFileSecurityW" << childFile << GetLastError();
+                return false;
+            }
+        }
+    }
+
+    if (!SetFileSecurityW(QDir::toNativeSeparators(path).toStdWString().c_str(), info, &newSecurityDescriptor)) {
+        qCWarning(lcFileSystem) << "error when calling SetFileSecurityW" << QDir::toNativeSeparators(path) << GetLastError();
+        return false;
+    }
+
+    return true;
+}
+
 #endif
 
 } // namespace OCC

diff --git a/src/common/filesystembase.h b/src/common/filesystembase.h
index 481e765f638f43e1d7b13b608366414a7184e54c..ef90aedb484856772a4c3ceb0dbf876b7d591136 100644 (file)
--- a/src/common/filesystembase.h
+++ b/src/common/filesystembase.h
@@ -176,6 +176,8 @@ namespace FileSystem {
     std::filesystem::perms OCSYNC_EXPORT filePermissionsWinSymlinkSafe(const QString &filename);
     std::filesystem::perms OCSYNC_EXPORT filePermissionsWin(const QString &filename);
     void OCSYNC_EXPORT setFilePermissionsWin(const QString &filename, const std::filesystem::perms &perms);
+
+    bool OCSYNC_EXPORT setAclPermission(const QString &path, FileSystem::FolderPermissions permissions, bool applyAlsoToFiles);
 #endif
 
     /**

diff --git a/src/libsync/filesystem.cpp b/src/libsync/filesystem.cpp
index 3b06ff1d72fe00e8ea2cbd143409b75cad699288..45a401ad39381d6272551a5f7aedbe0dc0ebcd23 100644 (file)
--- a/src/libsync/filesystem.cpp
+++ b/src/libsync/filesystem.cpp
@@ -363,144 +363,10 @@ bool FileSystem::setFolderPermissions(const QString &path,
     }
 
 #ifdef Q_OS_WIN
-    SECURITY_INFORMATION info = DACL_SECURITY_INFORMATION;
-    std::unique_ptr<char[]> securityDescriptor;
-    auto neededLength = 0ul;
-
-    if (!GetFileSecurityW(path.toStdWString().c_str(), info, nullptr, 0, &neededLength)) {
-        const auto lastError = GetLastError();
-        if (lastError != ERROR_INSUFFICIENT_BUFFER) {
-            qCWarning(lcFileSystem) << "error when calling GetFileSecurityW" << path << lastError;
-            return false;
-        }
-
-        securityDescriptor.reset(new char[neededLength]);
-
-        if (!GetFileSecurityW(path.toStdWString().c_str(), info, securityDescriptor.get(), neededLength, &neededLength)) {
-            qCWarning(lcFileSystem) << "error when calling GetFileSecurityW" << path << GetLastError();
-            return false;
-        }
-    }
-
-    int daclPresent = false, daclDefault = false;
-    PACL resultDacl = nullptr;
-    if (!GetSecurityDescriptorDacl(securityDescriptor.get(), &daclPresent, &resultDacl, &daclDefault)) {
-        qCWarning(lcFileSystem) << "error when calling GetSecurityDescriptorDacl" << path << GetLastError();
-        return false;
-    }
-    if (!daclPresent || !resultDacl) {
-        qCWarning(lcFileSystem) << "error when calling DACL needed to set a folder read-only or read-write is missing" << path;
-        return false;
-    }
-
-    PSID sid = nullptr;
-    if (!ConvertStringSidToSidW(L"S-1-5-32-545", &sid))
-    {
-        qCWarning(lcFileSystem) << "error when calling ConvertStringSidToSidA" << path << GetLastError();
-        return false;
-    }
-
-    ACL_SIZE_INFORMATION aclSize;
-    if (!GetAclInformation(resultDacl, &aclSize, sizeof(aclSize), AclSizeInformation)) {
-        qCWarning(lcFileSystem) << "error when calling GetAclInformation" << path << GetLastError();
-        return false;
-    }
-
-    const auto newAclSize = aclSize.AclBytesInUse + sizeof(ACCESS_DENIED_ACE) + GetLengthSid(sid);
-    qCDebug(lcFileSystem) << "allocated a new DACL object of size" << newAclSize;
-
-    std::unique_ptr<ACL> newDacl{reinterpret_cast<PACL>(new char[newAclSize])};
-    if (!InitializeAcl(newDacl.get(), newAclSize, ACL_REVISION)) {
-        const auto lastError = GetLastError();
-        if (lastError != ERROR_INSUFFICIENT_BUFFER) {
-            qCWarning(lcFileSystem) << "insufficient memory error when calling InitializeAcl" << path;
-            return false;
-        }
-
-        qCWarning(lcFileSystem) << "error when calling InitializeAcl" << path << lastError;
-        return false;
-    }
-
-    if (permissions == FileSystem::FolderPermissions::ReadOnly) {
-        qCInfo(lcFileSystem) << path << "will be read only";
-
-        if (!AddAccessDeniedAceEx(newDacl.get(), ACL_REVISION, OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE,
-                                  FILE_DELETE_CHILD | DELETE | FILE_WRITE_DATA | FILE_WRITE_ATTRIBUTES | FILE_WRITE_EA | FILE_APPEND_DATA, sid)) {
-            qCWarning(lcFileSystem) << "error when calling AddAccessDeniedAce << path" << GetLastError();
-            return false;
-        }
-    }
-
-    if (permissions == FileSystem::FolderPermissions::ReadWrite) {
-        qCInfo(lcFileSystem) << path << "will be read write";
-    }
-
-    for (int i = 0; i < aclSize.AceCount; ++i) {
-        void *currentAce = nullptr;
-        if (!GetAce(resultDacl, i, &currentAce)) {
-            qCWarning(lcFileSystem) << "error when calling GetAce" << path << GetLastError();
-            return false;
-        }
-
-        const auto currentAceHeader = reinterpret_cast<PACE_HEADER>(currentAce);
-
-        if (permissions == FileSystem::FolderPermissions::ReadWrite && (ACCESS_DENIED_ACE_TYPE == (currentAceHeader->AceType & ACCESS_DENIED_ACE_TYPE))) {
-            qCWarning(lcFileSystem) << "AceHeader" << path << currentAceHeader->AceFlags << currentAceHeader->AceSize << currentAceHeader->AceType;
-            continue;
-        }
-
-        if (!AddAce(newDacl.get(), ACL_REVISION, i + 1, currentAce, currentAceHeader->AceSize)) {
-            const auto lastError = GetLastError();
-            if (lastError != ERROR_INSUFFICIENT_BUFFER) {
-                qCWarning(lcFileSystem) << "insufficient memory error when calling AddAce" << path;
-                return false;
-            }
-
-            if (lastError != ERROR_INVALID_PARAMETER) {
-                qCWarning(lcFileSystem) << "invalid parameter error when calling AddAce" << path << "ACL size" << newAclSize;
-                return false;
-            }
-
-            qCWarning(lcFileSystem) << "error when calling AddAce" << path << lastError << "acl index" << (i + 1);
-            return false;
-        }
-    }
-
-    SECURITY_DESCRIPTOR newSecurityDescriptor;
-    if (!InitializeSecurityDescriptor(&newSecurityDescriptor, SECURITY_DESCRIPTOR_REVISION)) {
-        qCWarning(lcFileSystem) << "error when calling InitializeSecurityDescriptor" << path << GetLastError();
-        return false;
-    }
-
-    if (!SetSecurityDescriptorDacl(&newSecurityDescriptor, true, newDacl.get(), false)) {
-        qCWarning(lcFileSystem) << "error when calling SetSecurityDescriptorDacl" << path << GetLastError();
-        return false;
-    }
-
-    auto currentFolder = QDir{path};
-    const auto childFiles = currentFolder.entryList(QDir::Filter::Files);
-    for (const auto &oneEntry : childFiles) {
-        const auto childFile = QDir::toNativeSeparators(path + QDir::separator() + oneEntry);
-
-        const auto &childFileStdWString = childFile.toStdWString();
-        const auto attributes = GetFileAttributes(childFileStdWString.c_str());
-
-        // testing if that could be a pure virtual placeholder file (i.e. CfApi file without data)
-        // we do not want to trigger implicit hydration ourself
-        if ((attributes & FILE_ATTRIBUTE_SPARSE_FILE) != 0) {
-            continue;
-        }
-
-        if (!SetFileSecurityW(childFileStdWString.c_str(), info, &newSecurityDescriptor)) {
-            qCWarning(lcFileSystem) << "error when calling SetFileSecurityW" << childFile << GetLastError();
-            return false;
-        }
-    }
-
-    if (!SetFileSecurityW(QDir::toNativeSeparators(path).toStdWString().c_str(), info, &newSecurityDescriptor)) {
-        qCWarning(lcFileSystem) << "error when calling SetFileSecurityW" << QDir::toNativeSeparators(path) << GetLastError();
-        return false;
-    }
+    // current read-only folder ACL needs to be removed from files also when making a folder read-write
+    // we currently have a too limited set of authorization for files when applying the restrictive ACL for folders on the child files
+    setFileReadOnly(path, permissions == FileSystem::FolderPermissions::ReadOnly);
+    setAclPermission(path, permissions, permissions == FileSystem::FolderPermissions::ReadWrite ? true : false);
 
     permissionsDidChange = true;
 #else

diff --git a/test/testpermissions.cpp b/test/testpermissions.cpp
index fb16bb1e494090fe63e3776dea731f7159278fdd..ee4507c100f99c64c549584e06fbf6414aecd72f 100644 (file)
--- a/test/testpermissions.cpp
+++ b/test/testpermissions.cpp
@@ -434,11 +434,7 @@ private slots:
             currentLocalState = fakeFolder.currentLocalState();
             count++;
         }
-#if defined Q_OS_WINDOWS
-        QCOMPARE(count, 0);
-#else
         QCOMPARE(count, 2);
-#endif
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
     }